Skip to content

Conversation

Prabhuk
Copy link
Contributor

@Prabhuk Prabhuk commented Aug 22, 2025

Implement support for subcommands in OptTable to attain feature parity with cl.

Design overview: https://discourse.llvm.org/t/subcommand-feature-support-in-llvm-opttable/88098

Issue: #108307

@Prabhuk Prabhuk requested review from petrhosek and MaskRay August 22, 2025 20:55
@Prabhuk Prabhuk marked this pull request as ready for review August 27, 2025 22:05
@Prabhuk Prabhuk requested a review from cyndyishida as a code owner August 27, 2025 22:05
@Prabhuk Prabhuk requested a review from jh7370 August 27, 2025 22:05
@Prabhuk Prabhuk requested a review from PiJoules September 5, 2025 19:07
HelloSubOptTable T;
unsigned MissingArgIndex, MissingArgCount;

auto HandleMultipleSubcommands = [](const ArrayRef<StringRef> SubCommands) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const might not be needed since ArrayRef is already immutable I believe. Same with the other const ArrayRefs below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. PTAL.

Comment on lines 460 to 467
// The option CommandIDsOffset.
OS << ", ";
if (R.getValue("CommandGroup") != nullptr) {
std::vector<const Record *> CommandGroup =
R.getValueAsListOfDefs("CommandGroup");
CommandKeyT CommandKey;
for (const auto &Command : CommandGroup)
CommandKey.push_back(Command->getName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move this into its own function since it's reused below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. PTAL.

Comment on lines 60 to 62
for (auto SC : SubCommands) {
llvm::errs() << " `" << SC << "`\n";
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. PTAL.

Comment on lines 198 to 200
OptTable(const StringTable &StrTable,
ArrayRef<StringTable::Offset> PrefixesTable,
ArrayRef<Info> OptionInfos, bool IgnoreCase = false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could probably just add

ArrayRef<Command> Commands = {},
ArrayRef<unsigned> CommandIDsTable = {},

so you don't need to make another ctor. Same with other OpTables below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. PTAL.

}
if (SubCommands.size() == 1)
return SubCommands.front();
return SubCommand;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think SubCommand is still just {} here so maybe just return {}.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. PTAL.

Comment on lines +287 to +288
std::function<void(ArrayRef<StringRef>)> HandleMultipleSubcommands,
std::function<void(ArrayRef<StringRef>)> HandleOtherPositionals) const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add comments above briefly explaining what these callbacks are for. Mostly requesting since the implication to me (based on the example usage) is that a user might want to just throw an error and exit. If this is the primary/only desirable case, then that might be worth noting. I personally can't think of a reason one might not want to print help and exit, but maybe someone else might have a reason to. We could probably discourage that with comments or maybe saying the callback is NoReturn if we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. PTAL.

Comment on lines 564 to 569
for (const auto &C : Commands) {
if (FirstArg == C.Name) {
ActiveCommand = &C;
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Just a personal preference so feel free to ignore, but I think this looks cleaner

auto found = std::find_if(Commands.begin(), Commands.end(), [&](const auto &C){
  return FirstArg == C.Name
});
ActiveCommand = found == Commands.end() ? nullptr : *found;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, it looks like you have getActiveCommand below so maybe you could reuse it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reusing getActiveCommand. Using find_if in getActiveCommand implementation.
Done. PTAL.

// This loop prints subcommands list and sets ActiveCommand to
// TopLevelCommand while iterating over all commands.
for (const auto &C : Commands) {
if (C.Name == TopLevelCommandName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coud probably just do C.Name == "TopLevelCommandName" if it's not changed elsewhere and stick it as a constexpr global in OptTable.h if it's used elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 warning: result of comparison against a string literal is unspecified (use an explicit string comparison function instead) [-Wstring-compare]
  804 |         if (C.Name == "TopLevelCommand") {
      |                    ^  ~~~~~~~~~~~~~~~~~

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. PTAL.

assert((CurIndex == 0 || !Command.empty()) &&
"Only first command set should be empty!");
for (const auto &CommandKey : Command) {
auto It = llvm::find_if(Commands, [&](const Record *R) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use std::find_if directly. I think llvm::find_if just calls it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. PTAL.

@Prabhuk
Copy link
Contributor Author

Prabhuk commented Sep 15, 2025

@PiJoules -- Thank you for the review! I've addressed all the comments from the last version you reviewed. PTAL.

class HelloSubOptTable : public GenericOptTable {
public:
HelloSubOptTable()
: GenericOptTable(OptionStrTable, OptionPrefixesTable, InfoTable, false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thank you.

if (Subcommand.empty()) {
if (Args.hasArg(OPT_version)) {
llvm::outs() << "LLVM Hello Subcommand Example 1.0\n";
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

    if (Args.hasArg(OPT_version))
      llvm::outs() << "LLVM Hello Subcommand Example 1.0\n";

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thank you.


size_t OldSize = SubCommands.size();
for (const OptTable::Command &CMD : Commands) {
if (StringRef(CMD.Name) == "TopLevelCommand")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe extract "TopLevelCommand" into a header and make it a global rather than using it as a literal between different cpp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thank you. It's added to OptTable.h file

// Compile time representation for top level command (aka toolname).
// Offers backward compatibility with existing Option class definitions before
// introduction of commandGroup in Option class to support subcommands.
def TopLevelCommand : Command<"TopLevelCommand">;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if a user would define a Subcommand with the name "TopLevelCommand"? I'm assuming there should only be one with this name assuming it's like a sentinel. If only one should be defined, are there any checks that would assert this or diagnose if more than one was provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I'll add the check. Maybe it's worth changing the name to something that is probably not going to be useful as a user defined subcommand name. "TopLevelCommand" -> "TOPLEVELCOMMAND". Other option could be a "Command" type object with empty string for name to depict the top level command. I can make sure all the subcommands have a non empty string name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I did a refactoring which changes the design a bit. There is no abstraction for "TopLevelCommand" anymore which I always felt weird about maintaining. Now we only have subcommands that can be registered by the user. All regression tests pass and my llvm-hello-sub example behaves correctly. PTAL. I am working on adding more tests to cover subcommand use cases.

return internalParseOneArg(Args, Index, [VisibilityMask](const Option &Opt) {
return !Opt.hasVisibilityFlag(VisibilityMask);
});
return internalParseOneArg(Args, Index, nullptr,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/*ActiveCommand=*/nullptr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thank you.

unsigned FlagsToExclude) const {
return internalParseOneArg(
Args, Index, [FlagsToInclude, FlagsToExclude](const Option &Opt) {
Args, Index, nullptr,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thank you.

FlagsToExclude &= ~HelpHidden;
return internalPrintHelp(
OS, Usage, Title, ShowHidden, ShowAllAliases,
OS, Usage, Title, {}, ShowHidden, ShowAllAliases,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/*Subcommand=*/{}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thank you.

#define LLVM_OPTION_OPTTABLE_H

#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallSet.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smallset seems unused here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

#include "llvm/Option/Option.h"
#include "llvm/Support/Compiler.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/Error.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error seems unused here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


MissingArgIndex = MissingArgCount = 0;
unsigned Index = 0, End = ArgArr.size();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably exclude the newline

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 565 to 567
std::unique_ptr<Arg> A =
GroupedShortOptions ? parseOneArgGrouped(Args, Index)
: internalParseOneArg(Args, Index, ExcludeOption);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably can exclude this formatting change

Comment on lines 797 to 799
if (ID == ActiveSubCommandID) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single line if

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return std::find(SubCommandIDs.begin(), SubCommandIDs.end(), ActiveSubCommandID) != SubCommandIDs.end();

might be cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Thanks. Done.

Comment on lines 760 to 762
const SubCommand *ActiveSubCommand =
std::find_if(SubCommands.begin(), SubCommands.end(),
[&](const auto &C) { return Subcommand == C.Name; });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be misremembering, but find_if should return an iterator right?

Also is it always guaranteed here that ActiveSubCommand will be nonnull?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Updated the LHS to const auto. ActiveSubCommand could be null. There is an assert check to verify its not where we have "SubCommand" StringRef is not empty. PTAL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the return type to be the iterator type and updated the assert check accordingly. PTAL.

continue;

const Info &CandidateInfo = getInfo(Id);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove newline

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 82 to 84
for (auto SC : SubCommands) {
RSO1 << "\n" << SC;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single line loop

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 90 to 92
for (auto SC : Positionals) {
RSO1 << "\n" << SC;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

EXPECT_TRUE(AL.hasArg(OPT_uppercase));
EXPECT_FALSE(AL.hasArg(OPT_lowercase));
EXPECT_FALSE(AL.hasArg(OPT_version));
// Do not expect any error messages as this is a valid use case.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're using gtest, it might be more verbose if the comment was part of the error message on an expect failure, so something like:

EXPECT_EQ(std::string::npos, ErrMsg.find("Multiple subcommands passed")) << "Do not expect any error messages as this is a valid use case.";

same might be good for other comments above individual EXPECT lines below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thank you.

Copy link

github-actions bot commented Sep 29, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@PiJoules PiJoules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks

StringRef SubCommand) const {
assert(!SubCommand.empty() &&
"This helper is only for valid registered subcommands.");
typename ArrayRef<OptTable::SubCommand>::iterator SCIT =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably save a line and just use auto here since it's probably well known that std::find* functions return an iterator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. But it didn't save a line unfortunately :)

/// Return the string table used for option names.
const StringTable &getStrTable() const { return *StrTable; }

const ArrayRef<SubCommand> getSubCommands() const { return SubCommands; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ArrayRef is already immutable so const might not be needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// pairs.
std::map<std::string, std::vector<OptionInfo>> GroupedOptionHelp;

typename ArrayRef<OptTable::SubCommand>::iterator ActiveSubCommand =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably use auto here also

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Prabhuk
Copy link
Contributor Author

Prabhuk commented Oct 2, 2025

LGTM thanks

Thank you @PiJoules I'll wait for a day to see if there are any objections before landing.

@Prabhuk Prabhuk merged commit fdbd17d into llvm:main Oct 6, 2025
9 checks passed
quic-seaswara added a commit to qualcomm/eld that referenced this pull request Oct 6, 2025
this is for unblocking eld upstream :

llvm/llvm-project#155026

Signed-off-by: Shankar Easwaran <[email protected]>
quic-seaswara added a commit to qualcomm/eld that referenced this pull request Oct 6, 2025
this is for unblocking eld upstream :

llvm/llvm-project#155026

Signed-off-by: Shankar Easwaran <[email protected]>
Copy link
Member

@petrhosek petrhosek Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having a directory in examples dedicated to just subcommands, I think we should a directory for Option and subcommands should be just one of the examples in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll start a new PR for that change.

@llvm-ci
Copy link
Collaborator

llvm-ci commented Oct 7, 2025

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-darwin running on doug-worker-3 while building clang-tools-extra,clang,lld,llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/14465

Here is the relevant piece of the build log for the reference
Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'Clang-Unit :: ./AllClangUnitTests/17/48' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/Volumes/RAMDisk/buildbot-root/x86_64-darwin/build/tools/clang/unittests/./AllClangUnitTests-Clang-Unit-41567-17-48.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=48 GTEST_SHARD_INDEX=17 /Volumes/RAMDisk/buildbot-root/x86_64-darwin/build/tools/clang/unittests/./AllClangUnitTests
--

Note: This is test shard 18 of 48.
[==========] Running 509 tests from 107 test suites.
[----------] Global test environment set-up.
[----------] 1 test from MinimizeSourceToDependencyDirectivesTest
[ RUN      ] MinimizeSourceToDependencyDirectivesTest.DefineNumber
[       OK ] MinimizeSourceToDependencyDirectivesTest.DefineNumber (0 ms)
[----------] 1 test from MinimizeSourceToDependencyDirectivesTest (0 ms total)

[----------] 1 test from HeaderSearchTest
[ RUN      ] HeaderSearchTest.ShortenWithWorkingDir
[       OK ] HeaderSearchTest.ShortenWithWorkingDir (0 ms)
[----------] 1 test from HeaderSearchTest (0 ms total)

[----------] 1 test from ModuleDeclStateTest
[ RUN      ] ModuleDeclStateTest.ModuleInterfacePartition
[       OK ] ModuleDeclStateTest.ModuleInterfacePartition (1 ms)
[----------] 1 test from ModuleDeclStateTest (1 ms total)

[----------] 1 test from ParseHLSLRootSignatureTest
[ RUN      ] ParseHLSLRootSignatureTest.InvalidMissingRDParameterTest
[       OK ] ParseHLSLRootSignatureTest.InvalidMissingRDParameterTest (0 ms)
[----------] 1 test from ParseHLSLRootSignatureTest (1 ms total)

[----------] 1 test from ToolChainTest
[ RUN      ] ToolChainTest.VFSSolarisMultiGCCInstallation
[       OK ] ToolChainTest.VFSSolarisMultiGCCInstallation (4 ms)
[----------] 1 test from ToolChainTest (4 ms total)

[----------] 1 test from MultilibTest
[ RUN      ] MultilibTest.SelectSoftFP
[       OK ] MultilibTest.SelectSoftFP (0 ms)
[----------] 1 test from MultilibTest (0 ms total)

[----------] 2 tests from ExprMutationAnalyzerTest
[ RUN      ] ExprMutationAnalyzerTest.ReturnAsNonConstRRef
input.cc:1:47: warning: reference to stack memory associated with local variable 'x' returned [-Wreturn-stack-address]
    1 | int&& f() { int x; return static_cast<int &&>(x); }
      |                                               ^
[       OK ] ExprMutationAnalyzerTest.ReturnAsNonConstRRef (19 ms)
[ RUN      ] ExprMutationAnalyzerTest.SelfRef
[       OK ] ExprMutationAnalyzerTest.SelfRef (9 ms)
[----------] 2 tests from ExprMutationAnalyzerTest (28 ms total)

[----------] 1 test from LifetimeAnalysisTest
...

@llvm-ci
Copy link
Collaborator

llvm-ci commented Oct 7, 2025

LLVM Buildbot has detected a new failure on builder ppc64le-lld-multistage-test running on ppc64le-lld-multistage-test while building clang-tools-extra,clang,lld,llvm at step 12 "build-stage2-unified-tree".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/16524

Here is the relevant piece of the build log for the reference
Step 12 (build-stage2-unified-tree) failure: build (failure) (timed out)
...
724.765 [57/9/6658] Linking CXX executable unittests/Target/TargetMachineCTests
725.171 [57/8/6659] Linking CXX executable bin/llvm-split
725.172 [57/7/6660] Linking CXX executable bin/llc
725.682 [57/6/6661] Linking CXX executable bin/llvm-dwp
728.082 [57/5/6662] Linking CXX executable tools/lld/unittests/AsLibAll/LLDAsLibAllTests
728.501 [57/4/6663] Linking CXX executable bin/bugpoint
730.815 [56/4/6664] Linking CXX executable bin/opt
737.613 [56/3/6665] Linking CXX executable bin/lld
752.341 [56/2/6666] Building CXX object tools/bugpoint-passes/CMakeFiles/BugpointPasses.dir/TestPasses.cpp.o
755.454 [55/2/6667] Linking CXX shared module lib/BugpointPasses.so
command timed out: 1200 seconds without output running [b'ninja'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=1956.112859

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants